Skip to content

Add retries when setting maintainer-approval success check#5921

Open
janniklasrose wants to merge 1 commit into
mainfrom
janniklasrose/retry-setting-maintainer-approval
Open

Add retries when setting maintainer-approval success check#5921
janniklasrose wants to merge 1 commit into
mainfrom
janniklasrose/retry-setting-maintainer-approval

Conversation

@janniklasrose

Copy link
Copy Markdown
Contributor

Problem

The maintainer-approval.js script (run by the "PR approval" workflow) creates the required maintainer-approval status check on the PR head SHA when approval is satisfied:

await github.rest.checks.create({ ...checkParams, conclusion: "success", ... });

There is no verification that this write actually took effect. When checks.create returns 2xx but the check does not register on the head commit (GitHub eventual consistency), the workflow run still exits green while the required maintainer-approval status stays stuck as "Expected" in the UI. Because the pending path deliberately posts no check at all, nothing re-establishes it — the PR is silently blocked from the merge queue even though a maintainer approved, until some unrelated event happens to re-run the workflow.

This was observed on #5868: the "PR approval" run passed, a maintainer had approved, yet maintainer-approval remained "Expected" and the PR would not enter the merge queue.

Fix

Route all three success paths through a new createApprovalCheck helper that:

  1. Creates the success check.
  2. Re-reads the check on the head SHA via checks.listForRef to confirm it is visible.
  3. Retries up to 3 times (with a short backoff) if it has not registered.
  4. Calls core.setFailed if the check never lands, so the run goes red with an actionable message ("re-run this workflow to unblock the merge") instead of green-but-blocking.

A red, re-runnable check is far easier to notice and recover from than a silently stuck "Expected" status.

Tests

The test mock now models create → persist → read, with a flakyCreates knob to simulate non-persisting writes. Two new cases:

  • retries when the first write does not persist — second create lands, no failure.
  • fails when the check never persists — exhausts attempts and records setFailed.

The retry backoff is overridable via MAINTAINER_APPROVAL_VERIFY_DELAY_MS (set to 0 in tests). All 24 tests pass.

This pull request and its description were written by Isaac, an AI coding agent.

@github-actions

Copy link
Copy Markdown
Contributor

Waiting for approval

Based on git history, these people are best suited to review:

  • @simonfaltum -- recent work in .github/workflows/

Eligible reviewers: @andrewnester, @anton-107, @denik, @lennartkats-db, @pietern, @renaudhartert-db, @shreyas-goenka

Suggestions based on git history. See OWNERS for ownership rules.

@eng-dev-ecosystem-bot

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 50cb996

Run: 29321453138

Env 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 230 1084 5:31
💚​ aws windows 4 4 232 1082 7:54
💚​ aws-ucws linux 4 4 316 1001 5:18
💚​ aws-ucws windows 4 4 318 999 7:24
💚​ azure linux 4 4 230 1083 6:16
💚​ azure windows 4 4 232 1081 7:11
💚​ azure-ucws linux 4 4 318 998 5:58
💚​ azure-ucws windows 4 4 320 996 7:29
💚​ gcp linux 4 4 229 1085 5:08
💚​ gcp windows 4 4 231 1083 6:54
8 interesting tests: 4 RECOVERED, 4 SKIP
Test Name aws linux aws windows aws-ucws linux aws-ucws windows azure linux azure windows azure-ucws linux azure-ucws windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
Top 10 slowest tests (at least 2 minutes):
duration env testname
6:47 aws windows TestAccept
6:27 azure-ucws windows TestAccept
6:19 aws-ucws windows TestAccept
6:02 azure windows TestAccept
6:00 gcp windows TestAccept
3:00 gcp linux TestAccept
2:58 azure linux TestAccept
2:52 aws linux TestAccept
2:47 azure-ucws linux TestAccept
2:47 aws-ucws linux TestAccept

const delayMs = Number(process.env.MAINTAINER_APPROVAL_VERIFY_DELAY_MS ?? 2000);

for (let attempt = 1; attempt <= CHECK_VERIFY_ATTEMPTS; attempt++) {
await github.rest.checks.create({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checks.create doesn't seem to be idempotent, so retrying might create duplicate runs, should we do listForRef first and re-create if it's not there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants